Add CMake support #2280
Add CMake support #2280
Conversation
|
In case you want to test this out: # Build Mixxx
$ mkdir cmake_build
$ cd cmake_build
$ cmake -DCMAKE_INSTALL_PREFIX=/usr ..
$ cmake --build . --parallel 4
# Install to /usr
$ cmake --build . --target install
# Make a package
$ cmake --build . --target package
# Make a Debian package
$ cpack -G DEB
# Build and run tests
$ cmake --build . --parallel 4 --target mixxx-test --target test |
|
Thanks for working on this! Why does libebur128 need to be linked statically? |
|
Thank you for stating this big task. |
|
I updated the |
I changed the behaviour and added an option to enable static linking. By default, it now links libebur128 dynamically if it was found. If not, it's linked statically. |
|
The changes to .travis.yml somehow caused Travis to stop running on this PR. |
Fixed. Looks like the Travis YAML parser does not like commands like this: [ "$foo" = "$bar" ] && echo "baz"Travis can now build Mixxx using cmake, but a bunch of tests fail. Does anyone have an idea why that is? |
|
Travis is still on Xenial with CMake 3.5.1-1ubuntu3 not compatible with |
|
This is hard to say without a detailed log. Can you make the Travis log verbose? |
|
The verbose test output displayed in case a test fails:
4416 BaseTrackPlayerImpl::slotLoadTrack "[Channel1]"
4417 SoundSourceProxy - Unsupported file type: "file:///home/travis/build/mixxxdj/mixxx/src/test/sine-30.wav"
4418 SoundSourceProxy - No SoundSourceProvider for file "file:///home/travis/build/mixxxdj/mixxx/src/test/sine-30.wav"
4419 SoundSourceProxy - Unable to decode file "file:///home/travis/build/mixxxdj/mixxx/src/test/sine-30.wav"
4420 CachingReaderWorker - Failed to open file: "/home/travis/build/mixxxdj/mixxx/src/test/sine-30.wav"
4421 CachingReaderWorker - "[Channel1]" loadTrack() load failed for" "/home/travis/build/mixxxdj/mixxx/src/test/sine-30.wav" ", file invalid, unlocked reader lock
And then I hangs indefinitely until the test times out. Maybe I missed some library necessary for *.wav support? In any case it shouldn't hang forever until it's killed by cpack.
|
|
It is libsndfile |
|
@daschuer @Holzhaus Please don't introduce any workarounds for supporting Xenial! Support for Xenial should be dropped as soon as it requires any extra work. I only enabled it to verify that the build is still compatible with this ancient LTS version as Daniel claimed. Please switch the build to Bionic if needed. |
|
@daschuer Apparently it's possible to compile and run Mixxx' tests without defining |
|
I fully agree with @uklotzde. IMO it is not fair to ask other developers to do extra work for an outdated OS. Ubuntu 16.04 was released 3.5 years ago and has been outdated by another LTS release for 1.5 years. |
|
@uklotzde @Be-ing cb52d88 is the only change I did for xenial support. And checking if the policy actually exists is the right thing to do anyway IMO:
|
|
The windows build still fails, but I don't know why. I suppose this is the problem:
Maybe we can rename |
|
Is something else missing apart from fixing the windows build? |
|
Does the static linking of libebur128 work for you? It seems to work with the following diff, but I don't really understand it :) |
|
@rrrapha It works fine on my Linux machine (libebur128 is not installed). Can add this change and rerun? It prints the full path of the linked library. Maybe your set_target_properties(libebur128 PROPERTIES EXCLUDE_FROM_ALL TRUE)
add_dependencies(mixxx-lib libebur128)
target_include_directories(mixxx-lib PUBLIC
"${CMAKE_CURRENT_SOURCE_DIR}/lib/libebur128/ebur128"
)
target_link_libraries(mixxx-lib PUBLIC "${CMAKE_CURRENT_BINARY_DIR}/lib/libebur128/${CMAKE_STATIC_LIBRARY_PREFIX}ebur128${CMAKE_STATIC_LIBRARY_SUFFIX}")
+ message(STATUS "Linking: ${CMAKE_CURRENT_BINARY_DIR}/lib/libebur128/${CMAKE_STATIC_LIBRARY_PREFIX}ebur128${CMAKE_STATIC_LIBRARY_SUFFIX}")
else()
message(STATUS "Linking libebur128 dynamically")
target_link_libraries(mixxx-lib PUBLIC Ebur128::Ebur128)
endif() |
No, the path to the static library is correct. Are you sure it works correctly on Linux? |
Using CMake version 3.15.3: |
The issues was reported by rrrapha: mixxxdj#2280 (comment)
|
I pushed a fix (hopefully), although I still can neither reproduce the issue nor see why the current implementation could cause the behaviour you described. |
|
|
I think it is quite important to have a almost warning less build because if you are constantly bothered by as bunch of unimportant message, you will miss likely the one that is a hint for a real issue. On Xenial I have almost no warnings (need to check a full rebuild if this is still the case.) If you have new warnings from a new compiler, we should remove them. |
|
The warnings I posted above are from gcc 9.2.0 on Arch Linux. As soon as the code is warning-free we should enable |
|
Are there any other issues with this PR besides the warnings? Most of the warnings pop when building with scons as well. Of the 28 deprecated-copy warnings, only a single one is in the Mixxx code base, the other are in our copy of gmock. Can we update it? That might fix them. Regarding the other warnings: I'm no C++ expert but the restrict warnings look like they might indicate outright bugs in Mixxx. Maybe someone can have a look a them? I'll see if I can fix the other warnings. |
|
Just building. Can we clarify the use cases for the build types? empty, Debug, Release, RelWithDebInfo and MinSizeRel. I have just read "The default CMake build type is the “empty” build type, which is used in combination with the environment variables CFLAGS and CXXFLAGS." does it apply here as well? I think we should encourage users to use RelWithDebInfo because this produces debug info, required to be able to create a GDB backlog. What should we use for development? I like to use a fully optimized build with assertions enabled. What is the best way to build Mixxx with a single file at -0o? Is MinSizeRel a reasonable build type? |
It should, I didn't change anything in that regard.
Sounds reasonable.
Then you should probably use Another option might be to use
Some googling revealed that
|
|
Ok, so developer should build with BUILD_TYPE=debug. |
|
So from my side it LGTM. Give a hint if this is read to merge. |
|
@Holzhaus Please review my additions for Rekordbox. I had to mess around with the include directories to get the code (partially generated) compiled. Not particularly clean and interoperable. The compiler warnings will be fixed directly on master. |
|
I'm getting this during build: |
When an include directory for a target is marked as PUBLIC, it will also be added as to all targets that depend on it (i.e. if the target is a library, all targets that this library is linked to via target_link_libraries will also have this include directory). Hence, there's no need to set this include directory twice.
|
Feel free to merge if no other issues pop up. |
|
LGTM Last chance to express any concerns. Otherwise I will merge this branch in a few hours. |
|
I am looking forward to use it. It will be a real game changer for me. No more waiting for unnecessary rebuilds due to .sconsign.dblite issues. Thank you very much for this nice piece of work. :-) |
|
Ubuntu builds are failing now |
|
Ping. Builds are still failing. |
|
EDIT: Looks like |
|
Someone probably needs to add this to the build configuration:
|
|
Attempt to fix: #2361 |
This PR adds CMake support and aims to replace the SCons build system. This is still a work-in-progress, so maybe we can fix remaining issues collectively.
Building works on my Linux machine, but there are still issues.
Some of the
FindXXX.cmakemodules were not written by me and copied from other projects. They are under a more permissive license than Mixxx' GPL, so I think there shouldn't be a problem, but I thought I mention it in case this assumption is wrong.